Exposing opt-in double buffering for MG batched KMeans#2323
Exposing opt-in double buffering for MG batched KMeans#2323viclafargue wants to merge 4 commits into
Conversation
viclafargue
added
breaking
github-project-automation
Bot
moved this to Todo
in cuVS Library & Integrations Roadmap
| kmeans_params.batch_centroids = params.batch_centroids; | ||
| kmeans_params.init_size = params.init_size; | ||
| kmeans_params.streaming_batch_size = params.streaming_batch_size; | ||
| kmeans_params.streaming_batch_prefetch = params.streaming_batch_prefetch; |
There was a problem hiding this comment.
I really don't like the use of the term "streaming" here. It carries altogether too much baggage with it. Can we please just remove it here? I think we need to consider removing it from "streaming_batch_size" as well. Streaming to me means a k-means that runs through a streaming mechanism, meaning it can only go forward as an iterator and can restart from where it left off when more data is received. There are variants of k-means that satisfy this, but the batched variant we've implemented does not.
There was a problem hiding this comment.
The problem with batch_size is that we have two other parameters -- batch_centroids and batch_samples. So batch_size can be confusing. Perhaps we should reconsider the names of those two parameters then? Or maybe think of some other prefix for batch_size?
There was a problem hiding this comment.
What are the meanings of batch samples vs batch size?
There was a problem hiding this comment.
the tile sizes for the distance computation (local reductions are done on this tile) -- i.e. batch_samples * batch_centroids distances are computed at once. So I was saying that we are already doing what the first part of the flash-kmeans paper mentions (at least that was my understanding)
There was a problem hiding this comment.
/**
* batch_samples and batch_centroids are used to tile 1NN computation which is
* useful to optimize/control the memory footprint
* Default tile is [batch_samples x n_clusters] i.e. when batch_centroids is 0
* then don't tile the centroids
*
* NB: These parameters are unrelated to streaming_batch_size, which controls how many
* samples to transfer from host to device per batch when processing out-of-core
* data.
*/
int batch_samples = 1 << 15;
/**
* if 0 then batch_centroids = n_clusters
*/
int batch_centroids = 0;
There was a problem hiding this comment.
Let's get some benchmarks attached to this PR so that we can evaluate. Again , I suspect the actual device utilization is likely to cap long before the memory does, which would indicate that even if there's no gain in performance, so long as there is not a drop in performance, the simplicity for the user could be justified in the default. But let's back that up with numbers.
There was a problem hiding this comment.
BTW I'm not necessarily say we should or shouldn't make it the default, but I am saying we should look into the impact and provide real evidence either way (concrete numbers / formula always helps but benchmarks should ultimately guide us).
There was a problem hiding this comment.
If we are renaming the streaming_batch_size param, we must do it quick because its in the C layer. If we dont change it this release, it'll be locked user experience for the next 6 months due to ABI stability.
There was a problem hiding this comment.
Agree we should try to get this out in time for the release, but just in case it doesn't, we can always add the new parameter and handle it accordingly underneath until we are able to remove the old. Think about it like a deprecation.
There was a problem hiding this comment.
Just in case, I suppose we could always open up a smaller PR to do just the rename. Let's see how things go next week.
3 participants
Partially answers #2292
Prefetch is beneficial but not universal: it gives up to 1.21× speedup when pinned H2D copies can overlap KMeans work, but only ~1.09× in copy- or compute-limited cases and no gain for a single batch.